Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update JS API to better specify opaqueData exception identity #250

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dschuff
Copy link
Member

@dschuff dschuff commented Feb 15, 2023

No description provided.

Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! But I guess I still wasn't able to fully grasp what you mean by using the map of externaddr -> object you said in #248 (comment). Most of my questions in #248 (comment) still remain.

document/js-api/index.bs Show resolved Hide resolved
1. Set the [=surrounding agent=]'s [=associated store=] to |store|.
1. If |ret| is [=error=], throw an exception. This exception should be a WebAssembly {{RuntimeError}} exception, unless otherwise indicated by <a href="#errors">the WebAssembly error mapping</a>.
1. If |ret| is exception |exntag| |payload| |opaqueData|, then
1. If |ret| is exception |exntag| |payload|, then
1. Let |opaqueData| be the [=externref=] assocated with the returned exception.
1. If |opaqueData| is not [=ref.null=] [=externref=],
1. Let « [=ref.extern=] |externaddr| » be |opaqueData|.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this (preexisting) line mean we have the reverse mapping from opaqueData (or object) to externaddr..?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My interpretation of the preexisting line is that the core spec was expected to be updated to say that func_invoke (from the core spec) returns the opaqueData (and is also expected to say where opaqueData comes from). There is no need for a reverse mapping because opaqueData is the externaddr and is passed directly into this step. I think of this like as basically just being a cast from opaqueData (a construct that was to be defined in the core spec) to externaddr (a construct that is used in this spec as a key in the map).

1. Set the [=surrounding agent=]'s [=associated store=] to |store|.
1. If |ret| is [=error=], throw an exception. This exception should be a WebAssembly {{RuntimeError}} exception, unless otherwise indicated by <a href="#errors">the WebAssembly error mapping</a>.
1. If |ret| is exception |exntag| |payload| |opaqueData|, then
1. If |ret| is exception |exntag| |payload|, then
1. Let |opaqueData| be the [=externref=] assocated with the returned exception.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we retrieve this opaqueData from payload? This is what I was asking about in #248 (comment). Also what exactly is "the returned exception"? Is that opaqueData or a combination of a tag and a payload?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO "payload" is a separate concept from identity. Different exceptions (with different identity) can have the same payload, so the identity/opaqueData can't be uniquely identified from the payload. As written here, the exact mechanism used to track the identity is unspecified; it just says that exceptions have a unique identity and says where it needs to be created and preserved.

@@ -1081,7 +1083,7 @@ Note: Exported Functions do not have a \[[Construct]] method and thus it is not
1. Otherwise, if |resultsSize| is 1, return « [=?=] [=ToWebAssemblyValue=](|ret|, |results|[0]) ».
1. Otherwise,
1. Let |method| be [=?=] [=GetMethod=](|ret|, [=@@iterator=]).
1. If |method| is undefined, [=throw=] a {{TypeError}}.
1. If |method| is undefined, throw a {{TypeError}}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reason as above.

@@ -1113,7 +1115,7 @@ Note: Exported Functions do not have a \[[Construct]] method and thus it is not
1. Let |type| be the [=JavaScript exception tag=].
1. Let |payload| be « ».
1. Let |opaqueData| be [=ToWebAssemblyValue=](|v|, [=externref=])
1. [=WebAssembly/Throw=] with |type|, |payload| and |opaqueData|.
1. [=WebAssembly/Throw=] with |type|, |payload|, associating |opaqueData| with the thrown exception.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How? Also I'm not sure what exactly is "the thrown exception". Is that a combination of a tag and a payload? Do we have an object that represents that combination which can be used as a key to some map? (And where do we have that map? Don't we only have a map of externaddr -> object?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This "association" is defined a little more down in the definition of Throw below.
(but really it's the hand-wavy bit).
But I just realized I wrote the definition of throw as taking opaqueData as an argument, so there's no need to use the term "association" or "thrown" exception here. It should just say
[=WebAssembly/Throw=] with |type|, |payload|, and |opaqueData| as it did before, and then the definition down below should say what the association means. I'll make that update here, and put more replies down there.

document/js-api/index.bs Outdated Show resolved Hide resolved
1. Set the [=surrounding agent=]'s [=associated store=] to |store|.
1. If |ret| is [=error=], throw an exception. This exception should be a WebAssembly {{RuntimeError}} exception, unless otherwise indicated by <a href="#errors">the WebAssembly error mapping</a>.
1. If |ret| is exception |exntag| |payload| |opaqueData|, then
1. If |ret| is exception |exntag| |payload|, then
1. Let |opaqueData| be the [=externref=] assocated with the returned exception.
1. If |opaqueData| is not [=ref.null=] [=externref=],
1. Let « [=ref.extern=] |externaddr| » be |opaqueData|.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My interpretation of the preexisting line is that the core spec was expected to be updated to say that func_invoke (from the core spec) returns the opaqueData (and is also expected to say where opaqueData comes from). There is no need for a reverse mapping because opaqueData is the externaddr and is passed directly into this step. I think of this like as basically just being a cast from opaqueData (a construct that was to be defined in the core spec) to externaddr (a construct that is used in this spec as a key in the map).

@@ -1113,7 +1115,7 @@ Note: Exported Functions do not have a \[[Construct]] method and thus it is not
1. Let |type| be the [=JavaScript exception tag=].
1. Let |payload| be « ».
1. Let |opaqueData| be [=ToWebAssemblyValue=](|v|, [=externref=])
1. [=WebAssembly/Throw=] with |type|, |payload| and |opaqueData|.
1. [=WebAssembly/Throw=] with |type|, |payload|, associating |opaqueData| with the thrown exception.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This "association" is defined a little more down in the definition of Throw below.
(but really it's the hand-wavy bit).
But I just realized I wrote the definition of throw as taking opaqueData as an argument, so there's no need to use the term "association" or "thrown" exception here. It should just say
[=WebAssembly/Throw=] with |type|, |payload|, and |opaqueData| as it did before, and then the definition down below should say what the association means. I'll make that update here, and put more replies down there.

@@ -1361,10 +1363,14 @@ For any [=associated store=] |store|, the result of

To <dfn for=WebAssembly>throw</dfn> with a [=tag address=] |type|, a matching [=list=] of WebAssembly values |payload|, and an [=externref=] |opaqueData|, perform the following steps:

1. Associate |opaqueData| with the thrown exception.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But by the "thrown exception" I really mean the unique identity that the exception has as it propagates through the frames. The definition down below is what says that the identity needs to be preserved across a rethrow. Just a combination of tag+payload wouldn't be enough, because there could be more than one identical tag+payload (e.g. if an exception was thrown from JS, caught and swallowed in wasm, and then a fresh exception thrown from inside wasm with the same numeric payload; you would want to be able to tell the difference, it would have no stack trace, etc).
As you say, we only have a map of externaddr -> object, but because tag+payload isn't unique we can't have a mapping in this spec from tag+payload to externaddr (or any other form of unique identity).

Instead the implementation needs to create some kind of association between the propagating exception's identity and the opaqueData (i.e. its identity as defined in this spec). It does so in an unspecified way, that's the hand-wavy part: we just assume that this identity exists and can be tracked, and we specify that the identity is maintained across a rethrow, and that fresh throws start with a null identity.

Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be a bit confused by all this, but why is there a distinction between payload and opaque data?

The way I would have assumed this all to be modelled is as follows:

  1. The magic JS tag has a single parameter of type externref, which is the actual JS value. That way, the JS value can be obtained inside Wasm if need be.

  2. A Wasm exception with the magic JS tag is converted to the JS value itself when escaping to JS. Vice versa, any plain JS value thrown that is not a Wasm exception is wrapped into that tag when escaping from JS to Wasm.

  3. The mapping between the JS value and the Wasm extern address representing the exception's payload inside Wasm is what's stored in the map.

  4. For all other exceptions, i.e., proper Wasm exceptions, the inverse happens: they are wrapped into an JS value on the way out, and unwrapped on the way back in.

I believe that this way, we do not even need to "patch" the core spec (at least not to handle JS exceptions), since the identity of the JS value is preserved in terms of the externref payload.

Does this make sense?

That leaves the question whether we should also guarantee that a Wasm exception maintains its JS-side identity, once observed. Specifically, consider this sandwich example:

(module
  (tag $e)
  (func $g (import "m" "g"))
  (func $f (export "f) (throw $e))
  (func $h1 (export "h1")
    (call $g)  ;; calls $f
  )
  (func $h2 (export "h2")
    (try (call $g) (catch $e (rethrow 0)))  ;; calls $f
  )
)

let instance = new Instance(module, {m: {g}})
let exn

function g() {
  try {
    instance.exports.f()  // throws Wasm exception
  } catch (e1) {
    exn = e1
    throw e1
  }
}

try {
  instance.exports.h1()  // calls g
} catch (e2) {
  assert (e1 === e2)  // is this guaranteed to hold?
}

try {
  instance.exports.h2()  // calls g
} catch (e2) {
  assert (e1 === e2)  // is this guaranteed to hold?
}

To be honest, I'm not sure it's worth guaranteeing either. I don't see the use case, and it would not be easy to specify. In fact, it would be easier to say that this equivalence never holds (by creating a fresh JS wrapper each time a Wasm exception escapes).

@@ -1361,10 +1363,14 @@ For any [=associated store=] |store|, the result of

To <dfn for=WebAssembly>throw</dfn> with a [=tag address=] |type|, a matching [=list=] of WebAssembly values |payload|, and an [=externref=] |opaqueData|, perform the following steps:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the tag address referred to as a "type" here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll fix the tag/type wording in a separate change.

@Ms2ger
Copy link
Contributor

Ms2ger commented Feb 21, 2023

  1. The mapping between the JS value and the Wasm extern address representing the exception's payload inside Wasm is what's stored in the map.

As far as I understand, there is no Wasm extern address representing the exception's payload.

@rossberg
Copy link
Member

rossberg commented Feb 21, 2023

@Ms2ger, well, that's exactly what I'm suggesting, though: by exposing it as an externref, we'd naturally assign it such an address, and nothing else would be needed (AFAICS).

@dschuff
Copy link
Member Author

dschuff commented Feb 21, 2023

Regarding your numbered points 1-4 about the JS tag:
This is not the behavior that the current spec has (or that engines currently implement) e.g. currently we specify that the JS tag has an empty payload and can't be imported. But I actually agree that this behavior makes sense and is what we want (Heejin and I have actually been discussing adding a solution like this in response to user requests to allow the JS value to be obtained inside wasm just as you mentioned). So we should add that.

However, it's not actually what this change is trying to accomplish; this change is trying to accomplish your second question, of whether we should guarantee that a wasm exception maintains its JS-side identity.
The answer there is yes, we should, and the implementations currently shipping in web engines do.
(Heejin also recently added a test similar to your example).
This is because Error objects thrown from JS and WebAssembly.Exception objects created in JS (as emscripten does here) need to preserve their properties (especially their attached stack traces) as they propagate through wasm frames all the way out to outermost try/catch blocks or onerror handlers. (This use case doesn't strictly require that the objects be === but it does require that the properties are the same). Many users obviously depend on being able collect these stack traces regardless of whether they propagate through wasm frames after being thrown, or whether they originate from JS or emscripten code.

I agree that it is not easy to specify! That's why the existing opaqueData situation and this proposed amendment are so complicated and hand-wavy! But it's a very important property to have. And, it's apparently not difficult to implement. The first JS implementations actually had this identity property even before the spec said that they should.

Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for the clarification. Now I see what this PR is trying to accomplish. However, I think there's still a problem, see comment below.

Another thing I noticed and that should be fixed is that the text repeatedly refers to exception tags as "types" (including with the name of the [[Type]] property). But that is a category error, a tag is not a type. It is a (second-class) value which has a type. In particular, multiple different tags can have the same type. This spec is only concerned with tag values (i.e., identities), and does not care about their types.

(The view of tags as types probably originates from OO languages traditionally piggybacking exceptions on nominal class types, which provide an identity through their runtime class. But that's not what's happening here.)

@@ -1361,10 +1363,14 @@ For any [=associated store=] |store|, the result of

To <dfn for=WebAssembly>throw</dfn> with a [=tag address=] |type|, a matching [=list=] of WebAssembly values |payload|, and an [=externref=] |opaqueData|, perform the following steps:

1. Associate |opaqueData| with the identity of the thrown exception.
1. Unwind the stack until reaching the *catching try block* given |type|.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to assume that there is a catch block to handle it, but of course that does not need to be the case, and it is not when the exception actually escapes to JS.

Copy link
Member Author

@dschuff dschuff Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Ideally this text should probably say something like "create an exception and unwind the stack" according to the language of the core spec, and "do whatever the core spec says should be done with exceptions"; i.e. unwind until the exception is caught or propagate it back to the embedder. Is there a particular spot in the core spec that defines that creation?

Copy link
Member Author

@dschuff dschuff Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe what we actually need to do is define some kind of interface in the embedding appendix for that (that appendix at least needs to be updated to say what happens when an invoked function raises/propagates an exception). The question still remains though, since that interface will itself need to say what happens.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#268 is the beginning of what I meant by the above comment; although I noticed that it doesn't say anything about calls from the instance into the embedder, so there may not be anything relevant to the case we're talking about here.

1. Set the [=surrounding agent=]'s [=associated store=] to |store|.
1. If |ret| is [=error=], throw an exception. This exception should be a WebAssembly {{RuntimeError}} exception, unless otherwise indicated by <a href="#errors">the WebAssembly error mapping</a>.
1. If |ret| is exception |exntag| |payload| |opaqueData|, then
1. If |ret| is exception |exntag| |payload|, then
1. Let |opaqueData| be the [=externref=] assocated with the returned exception.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. Let |opaqueData| be the [=externref=] assocated with the returned exception.
1. Let |opaqueData| be the [=externref=] associated with the returned exception.

Though I think this wording is a bit insufficient. If the exception originates from a Wasm throw (and has not yet escaped to JS), then there is no externref associated with it yet, and you are actually have to create that association at this point. So I believe you need to distinguish this case (which should be the common case).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change down on line 1373 attempts to get at this. Those lines are the ones that really modify the behavior specified in the core spec, so it's not 100% obvious where they should go.
The rule that says "fresh exceptions created by wasm throw get null identities" could reasonably go here I think; but it's less obvious where the "identities propagate to JS" and "rethrows preserve identity" rules go, if not there.

I modified 1373 to try to make it a little more clear.

@dschuff dschuff marked this pull request as ready for review March 31, 2023 23:19
@titzer titzer mentioned this pull request Jul 24, 2023
@aheejin
Copy link
Member

aheejin commented Apr 24, 2024

Is this still relevant? If not, can we close this?

@dschuff
Copy link
Member Author

dschuff commented Apr 24, 2024

Is this still relevant? If not, can we close this?

It's not relevant for shipping exnref to phase4. We do still have the appendix with the "legacy" format. I haven't looked at it yet and don't know whether it also includes anything about the JS API but I was thinking it might still be worthwhile to say something about how it should work, since browsers will be the most likely engines to have support for it in the medium/long term. At the very least we should probably still have the legacy tests somewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants